-
Notifications
You must be signed in to change notification settings - Fork 49
feat: support multiple proof systems in STM #2689
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
c532b82
to
39fcd60
Compare
39fcd60
to
9dc8a47
Compare
9dc8a47
to
cf5d7fb
Compare
cf5d7fb
to
26f6e40
Compare
b62c4d8
to
51526d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces support for multiple proof systems in the STM (Stake-based Threshold Multisignature) implementation by adding an aggregate signature type parameter. The main change refactors the existing concatenation proof system into a modular structure that can support future proof systems.
- Adds
AggregateSignatureType
enum with initial support for Concatenation proof system - Refactors
AggregateSignature
from struct to enum to support multiple proof system implementations - Updates configuration, API, and infrastructure to include aggregate signature type parameter
Reviewed Changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
openapi.yaml | Adds aggregate_signature_type field to API specifications and examples |
mithril-stm/src/aggregate_signature/signature.rs | Refactors AggregateSignature from struct to enum with proof system abstraction |
mithril-stm/src/aggregate_signature/proof/ | Creates new modular proof system structure with concatenation implementation |
mithril-aggregator/src/configuration.rs | Adds aggregate signature type configuration parameter |
mithril-common/src/messages/aggregator_features.rs | Updates aggregator capabilities to include signature type |
mithril-test-lab/ | Updates test infrastructure to support aggregate signature type parameter |
Comments suppressed due to low confidence (2)
mithril-stm/src/aggregate_signature/signature.rs:1
- Use proper Rust formatting with a space after 'else'. Should be
} else {
instead of}else{
.
use std::collections::HashMap;
mithril-stm/src/aggregate_signature/signature.rs:1
- Use proper Rust formatting with a space after 'else'. Should be
} else {
instead of}else{
.
use std::collections::HashMap;
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
/// | ||
/// It returns an instance of `AggregateSignature`. | ||
/// Aggregate a set of signatures. | ||
#[deprecated(since = "0.6.0", note = "Use `aggregate_signatures_with_type` instead")] // TODO: Update since version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The TODO comment indicates the deprecation version needs to be updated. Either update the version number or remove the TODO comment if the version is correct.
#[deprecated(since = "0.6.0", note = "Use `aggregate_signatures_with_type` instead")] // TODO: Update since version | |
#[deprecated(since = "0.6.0", note = "Use `aggregate_signatures_with_type` instead")] |
Copilot uses AI. Check for mistakes.
51526d9
to
26d6768
Compare
26d6768
to
215e64e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
And create a new 'proof' sub-module where it is moved.
This will be removed when a new proof system is implemented. In the mean time it structures the code for ease of implementation later.
Instead of aggregate signature as tests are proof specific.
…igner aggregation
215e64e
to
94d39fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some minor comments, questions 😊
|
||
BlsSignature::verify_aggregate(msgp.as_slice(), &vks, &sigs)?; | ||
Ok(()) | ||
match self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A future proof system could require a different set of inputs to be verified. Is there a way to handle this case without causing a backward compatibility issue?
} | ||
|
||
#[cfg(test)] | ||
mod tests { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also include unit tests/proptests for some of the functions?
Even if we updated the tests in mod.rs
, it might be worth trying all possible outputs of the functions, WDYT?
KeyRegistration, Parameters, Signer, SingleSignature, SingleSignatureWithRegisteredParty, | ||
Stake, | ||
}; | ||
use crate::{AggregateSignatureType, bls_multi_signature::BlsVerificationKey}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason for importing AggregateSignatureType
here? Can we also do the following?
use crate::bls_multi_signature::BlsVerificationKey;
use crate::merkle_tree::MerkleBatchPath;
use crate::{
AggregateSignature, AggregateSignatureType, AggregationError, BasicVerifier, Clerk,
CoreVerifierError, Initializer, KeyRegistration, Parameters, Signer, SingleSignature,
SingleSignatureWithRegisteredParty, Stake,
};
/// The list of merkle tree indexes is used to create a batch proof, to prove that all signatures are from eligible signers. | ||
/// | ||
/// It returns an instance of `AggregateSignature`. | ||
#[deprecated(since = "0.5.0", note = "Use `aggregate_signatures` instead")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to update this deprecation message as well?
} | ||
} | ||
}else{ | ||
panic!("Unexpected aggregate signature type"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better way for handling this instead of panic
?
}; | ||
concatenation_proof.batch_proof = batch_proof; | ||
}else{ | ||
panic!("Unexpected aggregate signature type"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly
Err(AggregationError::UsizeConversionInvalid) => { | ||
println!("Invalid usize conversion"); | ||
} | ||
Err(AggregationError::UnsupportedProofSystem(aggregate_signature_type)) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In line 129, we still call the deprecated aggregate_signatures
function. Not a big deal, but it's better to update.
Content
This PR includes support for multiple proof systems in the STM library:
AggregateSignatureType
enum with initial support for Concatenation proof systemAggregateSignature
from struct to enum to support multiple proof system implementations (a new proof is now implemented in a dedicated sub-module of theproof
sub-module)Pre-submit checklist
Issue(s)
Closes #2680